Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(ecs): add codepipeline deploy-action to ecs cluster #2050

Merged

Conversation

aweiher
Copy link
Contributor

@aweiher aweiher commented Mar 19, 2019

No Unit-Test, one Integration Test ist present. The similar compontent s3 deploy-action has no unit-test (didnt found it), so I oriented there. Please give me feedback when required.

I have tested the integration test in my private AWS Account, and it worked.

Notes:

  • I did not implement toCodePipelineInvokeAction (on service?) as service and repository is required, as I didnt figured out an easy api for this. Need feedback if this is required.
  • The default IAM rules are maybe too open (ecs:* for *), I oriented on the default AWS CodePipeline Role: https://docs.aws.amazon.com/codepipeline/latest/userguide/how-to-custom-role.html
  • Not included in the component, but in the integration-test: sts:AssumeRole and iam:PassRole for * - same reason as point before. The following would be ideal, but I didnt figured out how to construct this with cdk:
              - Resource: "*"
                Effect: Allow
                Action:
                  - iam:PassRole
                Condition:
                  StringEqualsIfExists:
                    iam:PassedToService:
                      - ec2.amazonaws.com
                      - ecs-tasks.amazonaws.com

Fixes #1386


Pull Request Checklist

  • Testing
    • Unit test added (prefer not to modify an existing test, otherwise, it's probably a breaking change)
    • CLI change?: coordinate update of integration tests with team
    • cdk-init template change?: coordinated update of integration tests with team
  • Docs
    • jsdocs: All public APIs documented
    • README: README and/or documentation topic updated
  • Title and Description
    • Change type: title prefixed with fix, feat will appear in changelog
    • Title: use lower-case and doesn't end with a period
    • Breaking?: last paragraph: "BREAKING CHANGE: <describe what changed + link for details>"
    • Issues: Indicate issues fixed via: "Fixes #xxx" or "Closes #xxx"
  • Sensitive Modules (requires 2 PR approvers)
    • IAM Policy Document (in @aws-cdk/aws-iam)
    • EC2 Security Groups and ACLs (in @aws-cdk/aws-ec2)
    • Grant APIs (only if not based on official documentation with a reference)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.

@rix0rrr
Copy link
Contributor

rix0rrr commented Mar 20, 2019

@skinny85, @SoManyHs... opinions?

Copy link
Contributor

@skinny85 skinny85 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great @aweiher , a really high quality contribution! Just a few small comments/questions.

Thanks for taking the time to do this!

packages/@aws-cdk/aws-ecs/lib/base/pipeline-action.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-ecs/lib/base/pipeline-action.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-ecs/lib/base/pipeline-action.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-ecs/lib/base/pipeline-action.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-ecs/lib/base/pipeline-action.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-ecs/lib/base/pipeline-action.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-ecs/README.md Outdated Show resolved Hide resolved
@skinny85
Copy link
Contributor

The build failure seems like something transient. Can you rebase, and try again? (there are some conflicts with current master, so that would have to be done sometime anyway). Thanks!

@aweiher
Copy link
Contributor Author

aweiher commented Mar 22, 2019

Hej @skinny85 - Thank you very much for your code-review! :) I will fix the issues in the next days 👍

@skinny85
Copy link
Contributor

Hey @aweiher ,

there's been a pretty big revolution in the way the Actions are represented in code in PR #2098 . This might make rebasing this PR particularly challenging. Given how close to the final shape this PR is, I'd be willing to take it over from you - I was the one doing the changes in #2098, so I'm quite familiar with what needs to be changed in this new way of doing things.

Let me know!

@aweiher
Copy link
Contributor Author

aweiher commented Mar 31, 2019

Hi @skinny85 - No Problem, take this PR over. I really wanted to implement this by myself, but an other task is eating all my time unfortunately. I added one commit/comment regarding the permission, hope it helps.

@skinny85
Copy link
Contributor

skinny85 commented Apr 2, 2019

Thanks, will do!

@skinny85 skinny85 force-pushed the feature/codepipeline-ecs-deploy-action branch 2 times, most recently from b198296 to c73b7aa Compare April 5, 2019 20:21
@skinny85
Copy link
Contributor

skinny85 commented Apr 5, 2019

I've rebased the PR on top of master. @RomainMuller and @SoManyHs , I would appreciate if you took a look.

@skinny85 skinny85 force-pushed the feature/codepipeline-ecs-deploy-action branch from c73b7aa to 2ad6372 Compare April 5, 2019 21:30
@skinny85
Copy link
Contributor

skinny85 commented Apr 5, 2019

Missed a new feature in ECS when rebasing that changes the expected file for the integ tests.

@skinny85 skinny85 force-pushed the feature/codepipeline-ecs-deploy-action branch from 2ad6372 to 657411f Compare April 8, 2019 21:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort/medium Medium work item – several days of effort
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add ECS Deploy Action for Codepipeline
5 participants